-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data structures #14
Data structures #14
Conversation
56a34a2
to
a00d87f
Compare
@orbeckst Do you have any thoughts on the current content of the data-structures page: primarily ways to create Universes and AtomGroups, plus links to the Topology section for actions involving:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reads very nicely and I like the logical grouping. It's a lot of content, perhaps some of these pages can be split into smaller pages.
I'd also try to use sphinx linking as much as possible: use ref
instead of hard links, use intersphinx linking to numpy, Python, and the MDAnalysis docs (which should become the API docs, similar to what you have tested here).
See comments inline.
@orbeckst Unless you have any concerns I might merge this sometime late tomorrow (Australian) and ask the developer mailing list to comment. |
- source activate testenv | ||
- pip install --upgrade sphinx sphinx-sitemap nbsphinx sphinx_rtd_theme ipython | ||
- jupyter-nbextension enable nglview --py --sys-prefix | ||
script: | ||
- make -C ${SPHINX_DIR} html && touch ${HTML_DIR}/.nojekyll | ||
deploy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kill the DS Store file below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a top-level .gitignore
! You can copy the MDA .gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent stuff... just some nit-picking.
Notes on the notebooks (viewed via nbviewer)
- https://nbviewer.jupyter.org/github/MDAnalysis/UserGuide/blob/229e589b370c4285a0822eb76a58181cd0d7bb18/doc/source/examples/constructing_universe.ipynb#Creating-a-blank-Universe : the list is formatted as fixed width/"pre", should be normal text
- I do like the water example a lot. However, I wish you used a more realistic HOH angle (109º or whatever SPC has?) instead of 90º because invariably someone will copy&paste, even though we tell them not to. This guide will be around for a while so better get it right now. You can still generate coordinates on a grid by first making one molecule, putting them all in the same spot and then using translate() (or generate the grid with the translation vectors when making the initial array). Either way, this is already a medium/advanced tutorial so readers should be able to follow you.
P.S.: Extra cool: I can manipulate the image in the nbviewer static notebook. So awesome!!!! How did you do that?
General
I have various comments regarding "moving advanced stuff into extra sections and using links/footnotes". You can certainly just go ahead and merge and then potentially address these later. It would be great to get this PR merged.
Do you have something on the first page that says which minimum version of MDAnalysis this guide applies to? Also include the release of the guide and the date. (See MDAnalysisTutorial – I find all of this information very useful in assessing the relevance.)
- source activate testenv | ||
- pip install --upgrade sphinx sphinx-sitemap nbsphinx sphinx_rtd_theme ipython | ||
- jupyter-nbextension enable nglview --py --sys-prefix | ||
script: | ||
- make -C ${SPHINX_DIR} html && touch ${HTML_DIR}/.nojekyll | ||
deploy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a top-level .gitignore
! You can copy the MDA .gitignore
doc/source/standard_selections.rst
Outdated
|
||
The below names are drawn from the CHARMM 27, OPLS-AA, GROMOS 53A6, AMBER 03, and AMBER 99sb*-ILDN force fields. | ||
|
||
+------+------+------+------+------+------+------+------+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to generate this table programmatically from the MDAnalysis version that the UserGuide pertains to?
Although this is not likely to change soon, when it does I guarantee that someone will forget to update the UserGuide.
I think @jbarnoud has written code to, for instance, dynamically create the AUTHORS list in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that sticking it in conf.py
to run automatically would require installing MDAnalysis. If that isn't too heavy, this would be quite easy. Otherwise a script could be run manually from time-to-time to just regenerate standard_selections.rst
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least, you can have the table in a separate file that gets included here. This way, you can generate the table with a script without having to deal with the rest of the page in the script. I have an example in MDAnalysis/mdanalysis#2176; it is a bit of a monster, but mostly because of what gets written in the external file.
|
||
MDAnalysis will ignore everything after the ``*``. ``u.select_atoms("resname *E")`` will not select atoms whose residue name ends in E, but instead select every atom. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way that we only maintain this nice documentation of selections in a single place? If we update it in the code then we mustn't forget to update it here. (Not something to be solved in this PR but generally something to think about: How do we avoid duplicating docs that need to be updated.)
Trajectories | ||
=============== | ||
|
||
MDAnalysis groups dynamic data about a :code:`Universe` into its trajectory. This is typically loaded from a trajectory file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBD ;-)
* changed fragment example * added advanced topology section * now auto-generates standard selections
Thanks for the reviews @richardjgowers and @orbeckst, and for the pointer to the script @jbarnoud. I have now:
@orbeckst NGLView is cool! You can save the notebook widget state, and I think I had to include the javascript for widgets with this for it to render:
re: the MDAnalysis version -- I think ideally the UserGuide should apply to the most up-to-date one, with notes on when major changes happened? The tutorials should definitely have minimum versions on them. I haven't added them yet, but will once I work out which versions they work with. |
Sorry, my comments are too late to the party – but they were all basically 👍 . |
Universe
AtomGroup